chore: moving dev dependency to main dependency due to upgrade to ts 4 #460

obecny · 2021-04-28T19:07:09Z

Because of upgrading to ts v4 the types dependency needs to be now a part of main dependency, otherwise there is an error during installation for example

node_modules/@opentelemetry/instrumentation-pg/build/src/pg.d.ts:2:26 - error TS2307: Cannot find module 'pg' or its corresponding type declarations.

2 import * as pgTypes from 'pg';

We need to make a patch release asap

Flarna · 2021-04-28T19:13:22Z

Which installation are you referring here? I tried npm install @opentelemetry/instrumentation-pg but got no error.

dyladan · 2021-04-28T19:14:48Z

If you have a project which depends on @opentelemetry/instrumentation-pg but not on pg, it will fail to compile if you import PostgresInstrumentation.

This is not a common use-case, but it is at least the case for the instrumentations-all package which depends on all instrumentations.

Flarna · 2021-04-28T19:15:17Z

plugins/node/opentelemetry-instrumentation-aws-lambda/package.json

@@ -60,6 +59,7 @@
    "@opentelemetry/instrumentation": "^0.19.0",
    "@opentelemetry/resources": "^0.19.0",
    "@opentelemetry/semantic-conventions": "^0.19.0",
-    "@opentelemetry/tracing": "^0.19.0"
+    "@opentelemetry/tracing": "^0.19.0",
+    "@types/aws-lambda": "8.10.76"


Usually we don't pin dependencies, only dev-dependencies

I just moved them to different location, don't want to change them in any other way

I think we actually do pin all deps that aren't @opentelemetry deps usually iirc

The only dependency I found is semver e.g. here which is not pinned.

We should be careful which version we use for type definitions. If npm is not able to de-duplicate them users end up in having two versions of e.g. @types/aws-lambda in the same project which usually not what someone wants.

I doubt that typescript is able to use version A for one file and version B for other files. And clearly types will differ between versions.

We might want to think about using * if that is the case.

In this case, I think it's probably ok since we don't "leak" any of the types from the packages we wrap. If we use @types/pg@1 and the user has @types/pg@2, ours will simply be installed in the deep node_modules directory at something like node_modules/@opentelemetry/instrumentation-pg/node_modules/@types/pg no?

Hard to tell without trying.
Main question is what is typescript using - and remembering - when transpiling a file like this and the user has @types/pg installed in a significant different version then @opentelemetry/instrumentation-pg.

import * from "pg" import * from "@opentelemetry/instrumentation-pg"

I assume as long as the only types used are protected init(): (InstrumentationNodeModuleDefinition<typeof pgTypes> | InstrumentationNodeModuleDefinition<typeof pgPoolTypes>)[]; in an protected method never used by end users it's don't care.

Once there are APIs using more types it may get problematic.

maybe we should somehow hide/remove the types from InstrumentationAbstract#init to avoid this at all?

Guys you are aware that because we upgraded to ts 4 now all the packages doesn't contain the types and if you don't install the package by yrself it cannot be used. This change is just fixing to how it was, should be already merged and released. Then we can discuss about all other things but currently the packages are broken especially the meta package.

@obecny is right. this simple program fails to compile because of missing types currently:

import type {} from '@opentelemetry/auto-instrumentations-node';

A simple local test seems to show that typescript correctly differentiates the types. I manually installed @types/pg under node_modules/@opentelemetry/instrumentation-pg/node_modules/@types/pg and installed it also at node_modules/@types/pg. I made a breaking incompatible change to the types manually to the node_modules/@types/pg. My program still compiled successfuly and didn't break the checks for node_modules/@opentelemetry/instrumentation-pg so I think @Flarna's version concerns are OK.

How would that be a different if you keep types in dev and compile using ts 3.x ? you would still break it exactly the same ?. The files will be in the same location if you either keep in dev with ts3 or move it to main packages and compile with ver. 4. You will have exactly the same pinned version of types right ?

codecov · 2021-04-28T19:29:17Z

Codecov Report

Merging #460 (ad053c6) into main (45e8751) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #460   +/-   ##
=======================================
  Coverage   95.24%   95.24%           
=======================================
  Files         133      133           
  Lines        8252     8252           
  Branches      810      810           
=======================================
  Hits         7860     7860           
  Misses        392      392

dyladan · 2021-04-30T20:36:01Z

I can't figure out why this is caused. My first thought was that it was the upgrade to tsc 4, but the output from tsc 3 and the output from tsc 4 both contain the broken line import * as pgTypes from 'pg'.

I think this has likely always been like this, but because we never had metapackages that had real code before we never noticed. The failure is only triggered if you install the instrumentation and try to import it without having the module being instrumented installed.

I think this dependency solution is a stopgap. I think the real fix might be to not import the instrumented module in the instrumentation package.

obecny · 2021-04-30T20:40:05Z

I can't figure out why this is caused. My first thought was that it was the upgrade to tsc 4, but the output from tsc 3 and the output from tsc 4 both contain the broken line import * as pgTypes from 'pg'.

I think this has likely always been like this, but because we never had metapackages that had real code before we never noticed. The failure is only triggered if you install the instrumentation and try to import it without having the module being instrumented installed.

I think this dependency solution is a stopgap. I think the real fix might be to not import the instrumented module in the instrumentation package.

This is happening not only with metapackage try import just any package it will not compile unless you add a types. And you have to add types into main because if you don't then your final consumer / user of yr package will hit exactly the same error.

dyladan · 2021-04-30T20:49:28Z

I can't figure out why this is caused. My first thought was that it was the upgrade to tsc 4, but the output from tsc 3 and the output from tsc 4 both contain the broken line import * as pgTypes from 'pg'.
I think this has likely always been like this, but because we never had metapackages that had real code before we never noticed. The failure is only triggered if you install the instrumentation and try to import it without having the module being instrumented installed.
I think this dependency solution is a stopgap. I think the real fix might be to not import the instrumented module in the instrumentation package.

This is happening not only with metapackage try import just any package it will not compile unless you add a types. And you have to add types into main because if you don't then your final consumer / user of yr package will hit exactly the same error.

There is no reason to have @opentelemetry/instrumentation-pg without having pg so that's not really a bug.

dyladan · 2021-04-30T20:50:21Z

Can you change the title since this actually happens with older versions of typescript too (i checked)

Flarna · 2021-04-30T22:17:41Z

I tried to reproduce this but failed. Not sure what differs from my setup compared to yours but import * as i from "@opentelemetry/auto-instrumentations-node" doesn't cause any problems for me.

I see also no difference between typescript 3 and 4.

obecny · 2021-05-03T12:59:01Z

@Flarna @dyladan ok I will let you then figure out what is the best way to fix it. Bear in mind that currently the meta package doesn't work out of box so people won't be able to use it without fixing themselves.

dyladan · 2021-05-03T15:44:26Z

@obecny we are simply trying to make sure we understand the problem so we can actually have a correct fix instead of applying fixes without fully understanding the issue.

@Flarna can you try to npm i and npm test with the attached sample project? It definitely is failing on my machine.

tests.zip

Flarna · 2021-05-03T16:25:00Z

ok, can reproduce.

Reason why I haven't seen the problem before is that I did

tsc --init
tsc -p .

The generated tsconfig.json includes "skipLibCheck": true which avoids this problem.

dyladan · 2021-05-03T18:02:56Z

Now that we can reliably repro, do we think each plugin should declare the types package as a dependency, or should the metapackage declare them all? I would say the metapackage probably.

obecny · 2021-05-03T18:07:43Z

having this on each package will be working correctly in each case, the meta will receive "fixed" package automatically then. Or any other meta package that any 3rd party might already have

dyladan · 2021-05-03T18:19:57Z

I think that's fair. I dont have a strong opinion each way. I see you deleted your branch or I was about to reopen this PR

obecny · 2021-05-03T18:21:02Z

I think that's fair. I dont have a strong opinion each way. I see you deleted your branch or I was about to reopen this PR

it had conflicts anyway

Flarna · 2021-05-03T22:04:42Z

I think we should put it into each package. But I still think we should not pin the versions instead use * like it is done in dependencies within @types packages.
For some types it happens automatically anyway. For example@types/graphql version 14.5.0 is just a stub which itself depends on graphql version * which includes the types now.

An I still think we should aim mid term to get rid of "leaking" such dev dependencies.

dyladan · 2021-05-05T15:24:30Z

An I still think we should aim mid term to get rid of "leaking" such dev dependencies.

I agree here. We should be able to move all of the types we need into our packages directly instead of depending on them. It's a painful process but it removes this dependency issue.

Flarna · 2021-05-05T16:09:30Z

At least in most cases we leak only at one place, the protected init function of the Instrumentation. e.g. for pg: protected init(): (InstrumentationNodeModuleDefinition<typeof pgTypes> | InstrumentationNodeModuleDefinition<typeof pgPoolTypes>)[];

I don't think the base class actually needs the types. I have not tried yet but I think if we return InstrumentationNodeModuleDefinition<unknown> we could hide this.

obecny added the bug Something isn't working label Apr 28, 2021

obecny self-assigned this Apr 28, 2021

obecny requested a review from a team April 28, 2021 19:07

Flarna reviewed Apr 28, 2021

View reviewed changes

chore: moving dev dependency to main dependency due to upgrade to ts 4

ad053c6

obecny force-pushed the deps branch from 5549c4c to ad053c6 Compare April 28, 2021 19:20

obecny mentioned this pull request Apr 28, 2021

chore: updating to latest version api 1.0.0-rc.0 core 019 contrib 016 lightstep/otel-launcher-node#42

Merged

obecny closed this May 3, 2021

obecny deleted the deps branch May 3, 2021 18:05

Flarna mentioned this pull request Jun 4, 2021

chore: avoid leaking type definitions in instrumentations open-telemetry/opentelemetry-js#2256

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: moving dev dependency to main dependency due to upgrade to ts 4 #460

chore: moving dev dependency to main dependency due to upgrade to ts 4 #460

obecny commented Apr 28, 2021

Flarna commented Apr 28, 2021

dyladan commented Apr 28, 2021

Flarna Apr 28, 2021

obecny Apr 28, 2021

dyladan Apr 28, 2021

Flarna Apr 28, 2021

dyladan Apr 28, 2021

Flarna Apr 28, 2021

obecny Apr 30, 2021

dyladan Apr 30, 2021

dyladan Apr 30, 2021

obecny Apr 30, 2021

codecov bot commented Apr 28, 2021 •

edited

Loading

dyladan commented Apr 30, 2021

obecny commented Apr 30, 2021

dyladan commented Apr 30, 2021 •

edited

Loading

dyladan commented Apr 30, 2021

Flarna commented Apr 30, 2021

obecny commented May 3, 2021 •

edited

Loading

dyladan commented May 3, 2021

Flarna commented May 3, 2021

dyladan commented May 3, 2021

obecny commented May 3, 2021

dyladan commented May 3, 2021

obecny commented May 3, 2021

Flarna commented May 3, 2021

dyladan commented May 5, 2021

Flarna commented May 5, 2021

chore: moving dev dependency to main dependency due to upgrade to ts 4 #460

chore: moving dev dependency to main dependency due to upgrade to ts 4 #460

Conversation

obecny commented Apr 28, 2021

Flarna commented Apr 28, 2021

dyladan commented Apr 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 28, 2021 • edited Loading

Codecov Report

dyladan commented Apr 30, 2021

obecny commented Apr 30, 2021

dyladan commented Apr 30, 2021 • edited Loading

dyladan commented Apr 30, 2021

Flarna commented Apr 30, 2021

obecny commented May 3, 2021 • edited Loading

dyladan commented May 3, 2021

Flarna commented May 3, 2021

dyladan commented May 3, 2021

obecny commented May 3, 2021

dyladan commented May 3, 2021

obecny commented May 3, 2021

Flarna commented May 3, 2021

dyladan commented May 5, 2021

Flarna commented May 5, 2021

codecov bot commented Apr 28, 2021 •

edited

Loading

dyladan commented Apr 30, 2021 •

edited

Loading

obecny commented May 3, 2021 •

edited

Loading